Skip to content

MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills - #5574

Open
arcivanov wants to merge 1 commit into
MariaDB:bb-blob-main-montyfrom
arcivanov:MDEV-21879
Open

MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills#5574
arcivanov wants to merge 1 commit into
MariaDB:bb-blob-main-montyfrom
arcivanov:MDEV-21879

Conversation

@arcivanov

Copy link
Copy Markdown
Contributor

GROUP_CONCAT(DISTINCT x ORDER BY y) and JSON_ARRAYAGG(DISTINCT x ORDER BY y)
return a wrong answer once the duplicate filter runs out of memory.

The defect

Item_func_group_concat::add() decides whether a row is a duplicate by checking
whether Unique::elements_in_tree() grew after unique_add(). Unique flushes
its whole in-memory tree to disk when it runs out of memory, and
elements_in_tree() only counts what is still in memory, so after the first
flush that test says nothing about the rows already spilled.

MDEV-11563 made this harmless for GROUP_CONCAT(DISTINCT x) by building the
result in val_str() from unique_filter->walk(), which merges the spilled
parts back in. It left the ORDER BY case alone, where the result comes from the
sort tree that add() fills gated by the broken test.

Both directions of the failure are reachable, depending on how often the filter
flushes relative to the insert:

  1. Duplicates reach the result - 100 rows holding 50 distinct values give all
    100 values back.
  2. Rows are lost - 30 distinct rows of 2000 bytes give one value back.

The fix

add() no longer fills the sort tree when DISTINCT is used. val_str() walks
the merged unique_filter into the sort tree and then walks the sort tree, so the
rows are sorted after duplicate filtering is complete instead of during it.

Unique::walk() merges everything it flushed, so the sort tree can be handed more
rows than fit in memory. insert_to_order_tree() repacks it on the same memory
budget add() used, and a walk that runs out of memory sets result_cut so the
user gets a cut value warning rather than a silently short result.

This targets bb-blob-main-monty rather than main because it relies on
37077ccef15 "Limit the memory used by GROUP_CONCAT() with ORDER BY". Without
that commit's tree->allocated bound and its st.oom correction to
repack_tree(), pouring a spilled Unique into the sort tree trades a wrong
answer for an out-of-memory error.

Behaviour change

ORDER BY does not order rows that tie on the ordering expression, and which of
them comes first changes here. It used to follow the order the rows were read in;
it now follows the order the duplicate filter keeps them in. Unlike the old order,
the new one depends on neither the memory available nor the physical row order.
main.func_gconcat records one such tie and is re-recorded accordingly.

Testing

New main.gconcat_distinct_spill covers three things, and each assertion was
confirmed to fail before the fix and pass after, with the test unchanged between
the two runs:

  • the answer with the filter starved must equal the answer with memory to spare,
    for GROUP_CONCAT and JSON_ARRAYAGG, with and without ORDER BY
  • with the sort tree also starved so repack_tree() cuts the group, whatever
    comes back must still be deduplicated, still ordered, and still valid JSON
  • the tie order must not depend on the memory available or on the physical row
    order

Full main+heap suite: 1436/1436 pass.

`Item_func_group_concat::add()` decided whether a row was a duplicate
by checking whether `Unique::elements_in_tree()` had grown after
`unique_add()`:

    uint count= unique_filter->elements_in_tree();
    unique_filter->unique_add(get_record_pointer());
    if (count == unique_filter->elements_in_tree())
      row_eligible= FALSE;

`Unique` flushes its whole in-memory tree to disk when it runs out of
memory, and `elements_in_tree()` only counts what is still in memory.
After the first flush the test says nothing about the rows that were
already spilled.

**MDEV-11563** made this harmless for `GROUP_CONCAT(DISTINCT x)` by
building the result in `val_str()` from `unique_filter->walk()`, which
merges the spilled parts back in. It left the `ORDER BY` case alone.
There the result comes from the sort tree, which `add()` fills gated by
`row_eligible`, so the defect is still fully live.

Both directions of the failure are reachable, depending on how often
the filter flushes relative to the insert:

1. Duplicates reach the result. 100 rows holding 50 distinct values
   give all 100 values back.
2. Rows are lost. 30 distinct rows of 2000 bytes give one value back.

`JSON_ARRAYAGG(DISTINCT x ORDER BY y)` fails in the same way.

Fixed by not filling the sort tree from `add()` when `DISTINCT` is
used. `val_str()` now walks the merged `unique_filter` into the sort
tree and then walks the sort tree, so the rows are sorted after the
duplicate filtering is complete instead of during it.

`Unique::walk()` merges everything it flushed, so the sort tree can be
handed more rows than fit in memory. `insert_to_order_tree()` repacks
it on the same memory budget `add()` used, and a walk that runs out of
memory sets `result_cut`, so the user gets a cut value warning rather
than a silently short result.

**Behaviour change.** `ORDER BY` does not order rows that tie on the
ordering expression, and which of them comes first changes here. It
used to follow the order the rows were read in; it now follows the
order the duplicate filter keeps them in. Unlike the old order, the new
one depends on neither the memory available nor the physical row order.
`main.gconcat_distinct_spill` checks that, and `main.func_gconcat`
records one such tie.
@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Aug 20, 2026
@gkodinov gkodinov self-assigned this Aug 20, 2026

@gkodinov gkodinov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! This is a preliminary review.

LGTM. One small cleanup proposed.

Please stand by for the final review.

00,01,10,11,31
select group_concat(distinct a, c order by a) from t1;
group_concat(distinct a, c order by a)
00,01,11,10,31

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to stabilize this test instead of re-recording the new undeterministic order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is deterministic, it's just that determinism has changed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Behavior Change section please

@gkodinov
gkodinov requested a review from montywi August 20, 2026 08:01
@gkodinov gkodinov assigned montywi and unassigned gkodinov Aug 20, 2026
@gkodinov

Copy link
Copy Markdown
Member

BTW, any specific reason this is not based on 12.3 (the lowest affected version according to Jira)?

@arcivanov

Copy link
Copy Markdown
Contributor Author

BTW, any specific reason this is not based on 12.3 (the lowest affected version according to Jira)?

Yes, because it depends on other commits that are specifically in bb-blob-main-monty. I'll discuss this with @montywi if/when he is available. There may be further work around GROUP_CONCAT as well (e.g. mem -> HEAP -> Aria spillover mechanisms to eliminate the avoidable truncation of the results).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

3 participants